-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(typography): apply vanilla-extract #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- package 설정 수정
- module css 삭제 - vanilla-extract 변경된 코드 적용
- 테스트랑 스토리북 코드 수정
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
export const size = styleVariants(fontSize, (value) => ({ | ||
fontSize: `${value}px`, | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 callback으로 값 변환이 가능하구나!
lineHeight: value, | ||
})); | ||
|
||
export const typography = recipe({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
얘는 recipe로 처리해야할 만큼 복잡한 스타일은 아니라고 생각하는데, recipe를 사용하지 않는 방향으로 작성해볼 수 있을까??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알겠어!
나는 recipe 를 복잡하지않아도, 가독성이 좋아서 사용하는편인데 어떤 상황일 때 과한건지 판단하기가 어렵네..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 요건 기준을 한 번 맞춰봐도 좋겠네 ㅎㅎ 나는 라이브러리들은 가능하면 의존성이 없는 상태로 만들었으면 하는 바람이 있어!
이번 경우에도 recipe를 사용하지 않고 styleVariants를 사용해서 작성했을 때 구문을 이해하기 어려움이 없었기 때문에, 굳이 라이브러리를 사용해야할까 고민이 들었어 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오케 이해했오
수요일에 한번 이야기해볼까?
나도 생각해보면 라이브러리 의존성을 최대한 줄이는 게 적합하다고 생각해
export type LineHeight = keyof typeof lineHeightToken; | ||
|
||
export interface TypographyProps extends ComponentProps<'p'> { | ||
export interface TypographyProps extends Omit<ComponentProps<'p'>, 'color'> { | ||
as?: ElementType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as prop은 asChild랑 동일한 역할이라서 중복 기능의 구현으로 보이는데, 추가한 이유가 있을까?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as props 는 Typography 컴포넌트 자체가 특정 HTML 요소로 렌더링되도록 하고 싶을 때 사용해 h1, h2, span 등등?
asChild는 Typography 스타일을 기존의 다른 컴포넌트나 요소에 주입하거나, 불필요한 DOM 래퍼 없이 스타일을 적용하고 싶을 때 사용해.
- https://sandroroth.com/blog/react-polymorphic-components/
- https://www.radix-ui.com/primitives/docs/guides/composition
디자인 시스템 공부할 때 살펴본 문서인데, 약간의 차이때문에 구현을 통해 컴포넌트 자체의 활용도를 높일 수 있따 알고있어!
혹시 다른 의견있으면 말해줘도좋아!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 작성자는 구현 의도를 알 수 있지만, 디자인 시스템 사용자가 이런 의도를 구분할 수 있을까 걱정이 되는게 있어. 그래서 사용자에게 선택지를 많이 주기보다는 하나의 확실한 선택지만 제공하는 쪽이 낫지 않을까 생각해~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아! 이해했어.
사용하는 사람들이 "이 상황에서 어떤 것을 사용하는게 적합할까?" 라는 혼동을 최소한으로 하자는거지?
오케!! 다른 코드들의 패턴을 보니 asChild 를 활용해서 남겨둘게!
고마워!!!
- 코드 리뷰 반영 - recipe 제거 - typography.tsx 파일 리팩토링
- vanilla-extract dynamic 설치 - 코드 리뷰 반영
) { | ||
const Component = asChild ? Slot : 'p'; | ||
const Component = asChild ? Slot : as || 'p'; | ||
const dynamicStyles = color ? assignInlineVars({ [textColorVar]: color }) : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아주 사소한건데 color가 undefined일 때에도 빈 객체랑 동일한 결과물로 나오기 때문에 삼항연산자로 구분하지 않아도 괜찮을 것 같아 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 그렇구나
고마워!
- as props 삭제
Changes
Visuals
Checklist
Additional Discussion Points